Fixes #15247 | Update chat.cpp to support (at least) qwen3 reasoning + tool_choice = required #15248
+32
−3
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
All am I sure is that it fixes issues with Qwen3 + reasoning (enabled or disabled)+ tool calling. CC hermes2 contributor : @ochafik |
1 task
|
Back at the office on tuesday, re-reading the PR i might reconsider the logic around thinking_forced_open |
…already opened grammar)
…r accepting piece:`
|
For a future PR, the following functions needs the same kind of patch :
ready for review @ggerganov Not sure exactly who I should ping as ochafik seems to be busy this week |
ExtReMLapin
commented
Aug 25, 2025
This was referenced Aug 29, 2025
|
Not sure who I should ping |
|
Same comment as #15019 (comment) This is a smaller change, so I can take a look and merge this, but prefer if we have someone who would take over this part of the code. |
|
Got it, thanks for keeping me updated ! |
|
If you are being held hostage, blink twice @ochafik |
ochafik
suggested changes
Oct 30, 2025
There was a problem hiding this comment.
Thanks @ExtReMLapin ! Looks very promising :-)
Could you add some tests in test-chat.cpp (will need to force git add models/templates/qwen3-something.jinja, see models/templates/README.md )
Comment on lines
+2203
to
+2225
| builder.add_rule("thinking-start", "\"<think>\""); | ||
| builder.add_rule("thinking-content", "( [^<] | \"<\" [^/] | \"</\" [^t] | \"</t\" [^h] | \"</th\" [^i] | \"</thi\" [^n] | \"</thin\" [^k] | \"</think\" [^>] )*"); | ||
| builder.add_rule("thinking-end", "\"</think>\" space"); | ||
|
|
||
| //thinking grammar logic depending on if thinking_forced_open was to true (so already opened (and maybe closed)) and if thinking is even allowed | ||
| std::string thinking_grammar_logic = ""; // thinking tag was closed or not supported/wanted | ||
| if (extra_context["enable_thinking"]) { | ||
| data.grammar_triggers.push_back({ | ||
| COMMON_GRAMMAR_TRIGGER_TYPE_WORD, | ||
| data.thinking_forced_open ? "</think>" : "<think>" | ||
| }); | ||
| if (data.thinking_forced_open) { | ||
| //thinking tag was already opened by used so we don't need to add it again | ||
| thinking_grammar_logic = "(thinking-content thinking-end) "; | ||
| } | ||
| else | ||
| { | ||
| thinking_grammar_logic = "(thinking-start thinking-content thinking-end) "; | ||
| } | ||
| } | ||
|
|
||
|
|
||
| builder.add_rule("root", thinking_grammar_logic + (inputs.parallel_tool_calls ? "(" + tool_call + ")+" : tool_call)); |
There was a problem hiding this comment.
Here's (untested) code that addresses some issues:
- if tool_choice isn't required and we have a
</think>(if forced open) or a<think>, we're triggering the lazy grammar and requiring a tool call after it. - rules (apart from
root) can be renamed when colliding, gotta take the return value ofadd_rule
Suggested change
| builder.add_rule("thinking-start", "\"<think>\""); | |
| builder.add_rule("thinking-content", "( [^<] | \"<\" [^/] | \"</\" [^t] | \"</t\" [^h] | \"</th\" [^i] | \"</thi\" [^n] | \"</thin\" [^k] | \"</think\" [^>] )*"); | |
| builder.add_rule("thinking-end", "\"</think>\" space"); | |
| //thinking grammar logic depending on if thinking_forced_open was to true (so already opened (and maybe closed)) and if thinking is even allowed | |
| std::string thinking_grammar_logic = ""; // thinking tag was closed or not supported/wanted | |
| if (extra_context["enable_thinking"]) { | |
| data.grammar_triggers.push_back({ | |
| COMMON_GRAMMAR_TRIGGER_TYPE_WORD, | |
| data.thinking_forced_open ? "</think>" : "<think>" | |
| }); | |
| if (data.thinking_forced_open) { | |
| //thinking tag was already opened by used so we don't need to add it again | |
| thinking_grammar_logic = "(thinking-content thinking-end) "; | |
| } | |
| else | |
| { | |
| thinking_grammar_logic = "(thinking-start thinking-content thinking-end) "; | |
| } | |
| } | |
| builder.add_rule("root", thinking_grammar_logic + (inputs.parallel_tool_calls ? "(" + tool_call + ")+" : tool_call)); | |
| // thinking grammar logic depending on if thinking_forced_open was to true (so already opened (and maybe closed)) and if thinking is even allowed | |
| if (extra_context["enable_thinking"]) { | |
| data.grammar_triggers.push_back({ | |
| COMMON_GRAMMAR_TRIGGER_TYPE_WORD, | |
| data.thinking_forced_open ? "</think>" : "<think>" | |
| }); | |
| std::string prelude = ""; | |
| if (!data.thinking_forced_open) { | |
| prelude = builder.add_rule("think-start", "\"<think>\""); | |
| } | |
| prelude += " "; | |
| prelude += builder.add_rule("think-content", "( [^<] | \"<\" [^/] | \"</\" [^t] | \"</t\" [^h] | \"</th\" [^i] | \"</thi\" [^n] | \"</thin\" [^k] | \"</think\" [^>] )*"); | |
| prelude += " "; | |
| prelude += builder.add_rule("think-end", "\"</think>\" space"); | |
| prelude += " "; | |
| builder.add_rule("root", prelude + "(" + tool_call + ")" + (inputs.parallel_tool_calls ? "*" : "?")); | |
| } else { | |
| builder.add_rule("root", inputs.parallel_tool_calls ? "(" + tool_call + ")+" : tool_call); | |
| } |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was this issue that we would not use tool_choice at
requiredwith reasoning because of forced tool call imposed by grammar.Grammar now allows the model to think first.
Reasoning = big brain
Tool calling = strong arms
Now you can be very smart and very strong
Fixes #15247